-
-
Notifications
You must be signed in to change notification settings - Fork 147
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update Typescript and other dev(Dependencies) #440
Conversation
Code Climate has analyzed commit ebefe7b and detected 0 issues on this pull request. The test coverage on the diff in this pull request is 100.0% (50% is the threshold). This pull request will bring the total coverage in the repository to 90.6% (-1.6% change). View more on Code Climate. |
Codecov Report
@@ Coverage Diff @@
## master #440 +/- ##
==========================================
- Coverage 91.67% 90.22% -1.45%
==========================================
Files 139 139
Lines 12883 11293 -1590
Branches 2402 795 -1607
==========================================
- Hits 11810 10189 -1621
- Misses 1073 1104 +31
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #440 +/- ##
==========================================
- Coverage 91.76% 90.33% -1.44%
==========================================
Files 139 139
Lines 12861 11275 -1586
Branches 2396 793 -1603
==========================================
- Hits 11802 10185 -1617
- Misses 1059 1090 +31
Continue to review full report at Codecov.
|
@BBosman I was wondering where the decrease in coverage came from, so I ran the tests locally - for some reason there are 30 failing I am baffled by this.. why would these dependency upgrades have such side effect? :/ any idea what might be causing it? (btw, we can completely remove ajv dependency.. it was only added to pin it in the first place) |
@fkleuver Weird. I tested locally with But it appears we must find the reason in whatever is different between those setups and got an upgrade within this PR. (and I will remove |
@BBosman Well, running locally can be a bit tedious since it takes a few minutes. You can also just rely on CI. The coverage is supposed to stay identical. Fact that it went down is an immediate red flag that something is failing. Why the job doesn't report a failure is another issue and I'm not sure what's going on there. Seems to be running into some kind of timeout |
@fkleuver It keeps getting stranger. A fresh clone of the repo exhibits the same issue. git clone https://github.com/aurelia/aurelia.git aurelia2
cd aurelia2
npm run bootstrap
npm run build
npm run test Results in 30 failing tests. So my PR is surfacing the issue, but is not the root cause? |
Both |
Yeah and the build on master still succeeds. I don't know what's going on here, I'll assume it's a weird fluke and if it breaks on master then we'll take it from there. Hunting for ghosts like this is not the most productive way to work on vNext I think you would agree :) |
Pull Request
π Description
Upgraded to Typescript 3.3.
π« Issues
This is part of #425, which had too many moving parts, so I'm splitting it up.
π©βπ» Reviewer Notes
Notable changes:
package-lock.json
files)chromedriver
to~2.45.0
, as2.46.0
requires a newer version ofChrome
than the one that's running on CircleCI.ajv
, as NPM: 6.9.0 AJV Commit causing failure to execute node_modules (multiple dependencies affected i.e. ESLint and WebPack Loaders)Β ajv-validator/ajv#941 is fixed.π Test Plan
CircleCI
β Next Steps
Upgrade
Chrome
on CircleCI, so we can unpinchromedriver
.